ARCH-001 Phase 8: MerchandiseReturn controller/service consolidation (Admin/Store/Vendor) by KrzysztofPajak · Pull Request #799 · grandnode/grandnode2

KrzysztofPajak · 2026-08-27T20:27:22Z

Type: refactor

Issue

Grand.Web.Admin, Grand.Web.Store, and Grand.Web.Vendor each shipped their own
MerchandiseReturnController — the same class of duplication ARCH-001 already fixed for
Product (PR #790), Category (PR #792), Collection (PR #794), Order (PR #795),
Shipment (PR #796), and PaymentTransaction (PR #797). This is the 8th and final scoped entity.

Full design: docs/superpowers/specs/2026-08-26-arch001-merchandisereturn-consolidation-design.md
Full plan: docs/superpowers/plans/2026-08-26-arch001-merchandisereturn-consolidation.md
(Both gitignored under docs/superpowers/, same as every prior phase — on disk in this branch's
history but not tracked by git.)

MerchandiseReturn is a plain BaseEntity with flat StoreId/VendorId fields (not
IStoreLinkEntity, not a child-collection vendor scan like Order) — the simplest scope shape in
ARCH-001 so far. All three hosts have an identical, full CRUD+notes action surface (Vendor is not
a restricted subset here, unlike Order) — a single-level BaseMerchandiseReturnController was
sufficient, no management/read split.

Solution

GlobalAdminDataScope<MerchandiseReturn> reused unmodified for Admin (the entity carries an
unused SeId field but, unlike Order, no live Sales-Manager restriction was ever built on it) +
new bespoke StoreMerchandiseReturnDataScope/VendorMerchandiseReturnDataScope (flat-field
ownership checks) + a 3-branch, fail-closed RoutedMerchandiseReturnDataScope, all registered
centrally in Grand.Web.AdminShared/Startup/StartupApplication.cs.
Vendor's own IMerchandiseReturnViewModelService/MerchandiseReturnViewModelService had drifted
from AdminShared's shared implementation (Product-Phase-1 shape, not Category/Collection's
"Vendor never had one" shape) — missing attachment/IDownloadService support in notes, missing
per-store-scoped customer-email lookup. Reconciled onto AdminShared's service (adopting
Vendor's per-store email-lookup behavior — a disclosed behavior change for Admin/Store, see
Breaking changes), Vendor's duplicate deleted outright along with its two model files and the
now-dead HasAccessToMerchandiseReturn extension method.
Single-level BaseMerchandiseReturnController in Grand.Web.AdminShared holds all 9 actions
(List, GoToId, ProductsForMerchandiseReturn, Edit ×2, Delete, and the 3 notes actions). One
genuine, deliberately preserved host divergence: MerchandiseReturnNotesSelect's
Admin/Vendor-throws-ArgumentException vs Store-soft-denies-with-empty-Content("") — expressed
via a protected virtual NotFoundOrDeniedForNotesSelect() seam that Store's concrete subclass
overrides. (Written from scratch for this phase, not copied from Order's own
BaseOrderManagementController — that class turned out to have no equivalent host-differentiated
seam despite its own documentation implying one exists, a plan-authoring gap caught and corrected
during implementation, not carried forward here.)
Admin/Store/Vendor MerchandiseReturnControllers reduced to thin subclasses (each restating its
own host's [Area]/[Authorize*]/[AuthorizeMenu] attributes — see Testing for why this matters
more than usual on this PR).
Admin+Store's 7 views unified into Grand.Web.AdminShared/Views/AdminShared/MerchandiseReturn/
(area-parameterized via the existing ViewContext.RouteData.Values["area"] idiom), 11 widget-zone
call sites extracted into per-host satellites (Store's renamed store_merchandisereturn_* /
vc:store-widget, fixing the same "Store called vc:admin-widget, a tag helper never registered
in the Store app" dead-markup bug already found and fixed in every prior phase). Vendor's 6 views
stay host-specific (missing Documents/UserFields tabs, plain-text customer field, no
attachment UI) — a deliberate decision against partial unification, matching Order's own
precedent for Vendor's narrower view shape.
15-task plan executed via superpowers:subagent-driven-development, task-by-task review ledger
available on request.

Breaking changes

None to any public method signature. Disclosed, deliberate behavior changes, all judged safe:

MerchandiseReturnNoteAdd drops the caller-supplied orderId request parameter entirely.
Admin's original never validated that a caller-supplied orderId actually matched the
merchandise return being noted; Store's original did validate it but only by comparing against
a value it should never have accepted from the client in the first place. The unified action now
always resolves order server-side from merchandiseReturn.OrderId — the strictest of the three
original behaviors (matching Vendor's, which never took an orderId parameter at all), closing a
real if low-severity gap.
Per-store-scoped customer-email lookup adopted for Admin/Store. When
CustomerConfig.RegisterCustomersPerStore is enabled, searching the merchandise-return list by
customer email is now scoped to the current store (matching Vendor's original behavior and every
other already-consolidated per-store lookup) instead of searching across all stores.
Delete's ModelState.IsValid == false fallback branch removed. All three originals had an
Error(ModelState); return RedirectToAction("Edit", ...) branch for this case — practically
unreachable since Delete takes only a route id with no bound form fields that can fail
validation. Consolidated version always redirects to List on this branch, consistent with
"every access-denied redirect goes to List" elsewhere in this controller.

Testing

dotnet build GrandNode.sln — 0 errors, 4 pre-existing warnings unrelated to this diff (already
present on develop, none touching MerchandiseReturn files).
dotnet test src/Tests/Grand.Web.Admin.Tests (unfiltered) — 683 passed, 0 failed.
dotnet test src/Tests/Grand.Web.Store.Tests (unfiltered) — 42 passed, 0 failed.
dotnet test src/Tests/Grand.Web.Vendor.Tests (unfiltered) — 12 passed, 0 failed.
dotnet test src/Tests/Grand.Mapping.Tests (unfiltered) — 234 passed, 0 failed.
Live smoke test run against a real Kestrel-hosted instance + this developer's own local
MongoDB, with synthetic test data (4 merchandise returns covering a Store2-exclusive record, a
Store1-owned positive control, a "Vendor B"-exclusive record, and vendor1's own positive
control), driven as store1@store.com and vendor1@vendor.com:
- Caught and fixed a real Critical bug before this PR was safe to open: Admin/Store/Vendor's
  thin MerchandiseReturnController subclasses were missing [Area]/[Authorize*]/
  [AuthorizeMenu] — attributes that used to arrive transitively via each host's own base
  controller before consolidation, and which BaseMerchandiseReturnController (shared across
  hosts) correctly can't carry itself. Every MerchandiseReturn route 404'd on all three hosts
  until fixed. Exactly the failure class this live-test step exists to catch (unit tests
  construct the controller directly, bypassing MVC routing entirely, so no test in this plan
  could have caught it) — same bug class the Order phase's own Task 17 hit and fixed. Fixed,
  reviewed, and independently live-re-verified (302, not 404) before the rest of the smoke test
  ran.
- Store: List correctly scoped to Store1's own records; GoToId/Edit(GET)/
  ProductsForMerchandiseReturn/MerchandiseReturnNoteAdd/MerchandiseReturnNoteDelete all
  correctly deny cross-store access and succeed for Store1's own records;
  MerchandiseReturnNotesSelect on a denied record returns HTTP 200 with an empty body, not
  a throw — confirming the preserved soft-deny divergence live, not just in unit tests.
- Vendor: List correctly scoped to vendor1's own records; Edit(GET) correctly denies a
  different vendor's record and succeeds for vendor1's own; MerchandiseReturnNotesSelect on a
  denied record returns a genuine HTTP 500, server log confirming
  System.ArgumentException: No merchandise return found with the specified id thrown from
  NotFoundOrDeniedForNotesSelect() — confirming the Admin/Vendor-throw divergence live.
  Vendor's Edit view confirmed to show exactly 2 tabs (Info/Notes — no Documents/UserFields), no
  raw untranslated resource-key strings.
- Admin's own login could not be exercised live (a dev-DB credential mismatch unrelated to this
  phase); Admin's routing was independently confirmed live during the bug-fix re-verification
  step above (302, not 404), and GlobalAdminDataScope<MerchandiseReturn> is a trivially
  always-true implementation already covered by real (non-mocked) unit tests — judged the
  lowest-risk gap to leave live-unverified, disclosed here rather than silently skipped.
- Found, but not fixed as out-of-scope: a pre-existing latent NullReferenceException in
  Grand.Business.Messages.Commands.Handlers.Messages.GetMerchandiseReturnTokensCommandHandler
  (unconditional PickupAddress.CountryId dereference) when building a note-notification email
  for a merchandise return with no PickupAddress set — reachable in real operation whenever
  OrderSettings.MerchandiseReturns_AllowToSpecifyPickupAddress is disabled (it is, on this dev
  DB), not just via this session's synthetic test data. Untouched business-layer code, no task in
  this plan's diff goes near it — flagging for a separate fix, not blocking this PR.
- All synthetic test data deleted after testing; DB confirmed restored to pre-test state.
Executed via superpowers:subagent-driven-development: 15 plan tasks + 1 critical live-found
bug fix + 1 final-review fix round (1 Important + 2 Minor addressed: a missing order == null
guard in MerchandiseReturnNoteAdd that could persist a note before a downstream 500 on a
merchandise return whose order is missing — restored, matching Admin/Store's original strictest
behavior; a dead orderId field removed from the shared notes view's AJAX payload; the routing
regression tests extended to cover [AuthorizeMenu] too, closing the gap the live-found bug
opened) — all independently re-verified clean. Task-by-task review ledger available on request.

🤖 Generated with Claude Code

…turnDataScope per task review Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013XGyrBT97Wr9zjxgDmG2KR

…inDataScope<MerchandiseReturn> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013XGyrBT97Wr9zjxgDmG2KR

…andiseReturns storeId/vendorId, fold per-store customer-email lookup Admin/Store/Vendor's own MerchandiseReturnController.cs files still reference the old method name and will not build until Task 8's thin-subclass cutover replaces them - expected, not a regression. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013XGyrBT97Wr9zjxgDmG2KR

… region Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013XGyrBT97Wr9zjxgDmG2KR

… Admin/Store MerchandiseReturnController call sites Task 4 renamed IMerchandiseReturnViewModelService.PrepareReturnReqestListModel to PrepareReturnRequestListModel but left two callers on the old misspelled name, breaking the Grand.Web.Admin and Grand.Web.Store builds (and any test project depending on them). No behavioral change - these controllers are still replaced wholesale in Task 10. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013XGyrBT97Wr9zjxgDmG2KR

…MerchandiseReturnController

…troller

…in/Vendor-throw vs Store-soft-deny seam Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013XGyrBT97Wr9zjxgDmG2KR

…r to thin subclasses Deletes Vendor's drifted duplicate IMerchandiseReturnViewModelService/MerchandiseReturnViewModelService and its two model files, HasAccessToMerchandiseReturn (no remaining callers), and rebinds Vendor's views onto Grand.Web.AdminShared's model types via _ViewImports.cshtml aliases. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013XGyrBT97Wr9zjxgDmG2KR

….Web.AdminShared, extract widget-zone satellites (store_merchandisereturn_* naming for Store) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013XGyrBT97Wr9zjxgDmG2KR

…eturn removal regression guard, and SearchMerchandiseReturns storeId/vendorId forwarding test

…ibute tests - Created MerchandiseReturnControllerAttributeTests.cs with 5 tests - Verifies Admin, Store, and Vendor controllers all derive from BaseMerchandiseReturnController - Confirms base class has required PermissionAuthorizeAttribute for MerchandiseReturns - Confirms base class has AutoValidateAntiforgeryTokenAttribute - Uses extern alias for cross-host type references (StoreHost, VendorHost) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013XGyrBT97Wr9zjxgDmG2KR

…n/Store/Vendor MerchandiseReturnController BaseMerchandiseReturnController only carries [PermissionAuthorize]+[AutoValidateAntiforgeryToken] (shared across hosts); each concrete host subclass must restate [Area]/[AuthorizeAdmin|Store|Vendor]/ [AuthorizeMenu] itself, same pattern already fixed for Order's identical Task-17 defect. Confirmed live: /Store/MerchandiseReturn/List 404'd before this fix (real Kestrel instance against the real dev DB), no longer 404s after. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…dd, drop dead orderId from notes AJAX payload, extend attribute regression guard to AuthorizeMenu Final whole-branch review finding: dropping the caller-supplied orderId parameter (an approved, intentional security fix) had silently also dropped the null-check on the server-resolved order, letting a note persist before a downstream NRE on a merchandise return whose order is missing - Admin/Store's original controllers both guarded against this. Restored. Also removes the now-unbound orderId field the shared view still posted, and extends the routing-attribute regression tests (added for the earlier live-found Area/Authorize bug) to cover AuthorizeMenu too. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

+
+        var view = result as ViewResult;
+        Assert.IsNotNull(view);
+        Assert.AreSame(model, view.Model);


+
+        var redirect = result as RedirectToActionResult;
+        Assert.IsNotNull(redirect);
+        Assert.AreEqual("List", redirect.ActionName);


+
+        var redirect = result as RedirectToActionResult;
+        Assert.IsNotNull(redirect);
+        Assert.AreEqual("List", redirect.ActionName);


+
+        var redirect = result as RedirectToActionResult;
+        Assert.IsNotNull(redirect);
+        Assert.AreEqual("List", redirect.ActionName);


+
+        var redirect = result as RedirectToActionResult;
+        Assert.IsNotNull(redirect);
+        Assert.AreEqual("List", redirect.ActionName);


+
+        var redirect = result as RedirectToActionResult;
+        Assert.IsNotNull(redirect);
+        Assert.AreEqual("Edit", redirect.ActionName);


+
+        var json = result as JsonResult;
+        Assert.IsNotNull(json);
+        var gridModel = (DataSourceResult)json.Value;


+
+        var json = result as JsonResult;
+        Assert.IsNotNull(json);
+        var gridModel = (DataSourceResult)json.Value;


+        var result = await _controller.ProductsForMerchandiseReturn("mr1", new DataSourceRequest());
+
+        var json = result as JsonResult;
+        var gridModel = (DataSourceResult)json.Value;


+
+        var redirect = result as RedirectToActionResult;
+        Assert.IsNotNull(redirect);
+        Assert.AreEqual("List", redirect.ActionName);


KrzysztofPajak and others added 17 commits August 26, 2026 20:31

feat(arch001): add StoreMerchandiseReturnDataScope

6161ae1

fix(arch001): remove redundant CanView override on StoreMerchandiseRe…

4e7620e

…turnDataScope per task review Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013XGyrBT97Wr9zjxgDmG2KR

feat(arch001): add VendorMerchandiseReturnDataScope

50ba051

feat(arch001): add RoutedMerchandiseReturnDataScope and register IAdm…

52437be

…inDataScope<MerchandiseReturn> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013XGyrBT97Wr9zjxgDmG2KR

feat(arch001): add BaseMerchandiseReturnController skeleton with List…

0b499fa

… region Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013XGyrBT97Wr9zjxgDmG2KR

feat(arch001): add GoToId/ProductsForMerchandiseReturn region to Base…

7bec783

…MerchandiseReturnController

feat(arch001): add Edit (GET+POST) region to BaseMerchandiseReturnCon…

6eaad3c

…troller

feat(arch001): add Delete region to BaseMerchandiseReturnController

ad6c4e3

feat(arch001): add merchandise return notes region with preserved Adm…

d20dccb

…in/Vendor-throw vs Store-soft-deny seam Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013XGyrBT97Wr9zjxgDmG2KR

test(arch001): add cross-scope-type coverage, HasAccessToMerchandiseR…

e53e12e

…eturn removal regression guard, and SearchMerchandiseReturns storeId/vendorId forwarding test

Copilot AI lite review requested due to automatic review settings August 27, 2026 20:27

Copilot AI reviewed Aug 27, 2026

github-code-quality Bot found potential problems Aug 27, 2026

View reviewed changes

Navigation Menu

Uh oh!

ARCH-001 Phase 8: MerchandiseReturn controller/service consolidation (Admin/Store/Vendor) - #799

ARCH-001 Phase 8: MerchandiseReturn controller/service consolidation (Admin/Store/Vendor)#799
KrzysztofPajak wants to merge 17 commits into
developfrom
arch001/phase8-merchandisereturn-consolidation

KrzysztofPajak commented Aug 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

KrzysztofPajak commented Aug 27, 2026

Issue

Solution

Breaking changes

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants